Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Counter performance increase by incrementing atomics immediately when no attributes are provided #1563

Closed
wants to merge 6 commits into from
Closed

Counter performance increase by incrementing atomics immediately when no attributes are provided #1563

wants to merge 6 commits into from

Conversation

KallDrexx
Copy link
Contributor

Fixes #
Design discussion issue (if applicable) #

Changes

Previously, atomics were managed by the ValueMap itself in the measure() function. This meant that every counter.add(1, &[]) call would have to go through several boxed function calls, plus the non-zero cost of creating an empty attribute set. Moving the incrementing of atomics in the no attribute into the SDK's counter implementation directly bypasses all of these costs for significantly faster counter addition.

Since logic for managing these atomics was moved into multiple call sites, the original AtomicTracker trait was renamed to AtomicValue to better represent what the trait is calling. A new AtomicTracker struct was created to manage both the AtomicValue and an AtomicBool so that aggregations know if they have a no-attribute value that should be exported or not.

Benchmarks

main:

Counter/AddNoAttrs      time:   [29.452 ns 29.506 ns 29.559 ns]
Found 10 outliers among 100 measurements (10.00%)
  2 (2.00%) low severe
  3 (3.00%) low mild
  1 (1.00%) high mild
  4 (4.00%) high severe

Counter/AddNoAttrsDelta time:   [29.383 ns 29.513 ns 29.678 ns]
Found 12 outliers among 100 measurements (12.00%)
  6 (6.00%) low mild
  2 (2.00%) high mild
  4 (4.00%) high severe

Counter/AddOneAttr      time:   [139.60 ns 140.23 ns 141.07 ns]
Found 11 outliers among 100 measurements (11.00%)
  5 (5.00%) low mild
  3 (3.00%) high mild
  3 (3.00%) high severe

Counter/AddOneAttrDelta time:   [142.81 ns 143.27 ns 143.87 ns]
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

This PR:

Counter/AddNoAttrs      time:   [6.9870 ns 7.0010 ns 7.0183 ns]
                        change: [-76.475% -76.331% -76.213%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  5 (5.00%) high mild
  8 (8.00%) high severe
Counter/AddNoAttrsDelta time:   [6.9716 ns 6.9944 ns 7.0209 ns]
                        change: [-76.366% -76.236% -76.096%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  1 (1.00%) low severe
  3 (3.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe
Counter/AddOneAttr      time:   [139.78 ns 140.11 ns 140.45 ns]
                        change: [-0.2692% +0.4519% +1.3838%] (p = 0.28 > 0.05)
                        No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe
Counter/AddOneAttrDelta time:   [140.96 ns 141.41 ns 141.85 ns]
                        change: [-2.1149% -1.5107% -0.9781%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 16 outliers among 100 measurements (16.00%)
  4 (4.00%) low severe
  4 (4.00%) low mild
  8 (8.00%) high mild

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@KallDrexx KallDrexx requested a review from a team February 20, 2024 14:40
@lalitb
Copy link
Member

lalitb commented Feb 21, 2024

"@KallDrexx, nit - do you think including the no-attribute scenario text in the title would provide more clarity on what this PR is targeting?

@KallDrexx KallDrexx changed the title Counter performance increase by incrementing atomics immediately Counter performance increase by incrementing atomics immediately when no attributes are provided Feb 21, 2024
@cijothomas
Copy link
Member

"@KallDrexx, nit - do you think including the no-attribute scenario text in the title would provide more clarity on what this PR is targeting?

Will also include some recommendations in the docs. (I'll make them soon)

@codecov Codecov
Copy link

codecov bot commented Mar 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.41985% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 71.4%. Comparing base (348ec9e) to head (ace8eff).
Report is 10 commits behind head on main.

Files Patch % Lines
opentelemetry-sdk/src/metrics/pipeline.rs 80.0% 5 Missing ⚠️
opentelemetry-sdk/src/metrics/internal/sum.rs 93.7% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##            main   #1563     +/-   ##
=======================================
+ Coverage   71.0%   71.4%   +0.4%     
=======================================
  Files        135     136      +1     
  Lines      20751   20877    +126     
=======================================
+ Hits       14746   14920    +174     
+ Misses      6005    5957     -48

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cijothomas
Copy link
Member

The change itself is good, and nicely optimized the 0 attribute case to be as fast as possible. I'll not be merging this PR as-is, as I am doing some refactor on the Metrics SDK.

Discussed with original author @KallDrexx - as part of refactoring, the ideas from this PR will be folded into that PR. Many thanks for working on this. This is a good solution for high perf needs until we get the full notion of bound instruments.

@cijothomas cijothomas closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lalitb lalitb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KallDrexx @lalitb @cijothomas